Implement DFW settings Exclusion list #1037
Conversation
ksamoray
force-pushed
the
dfw_exclude_list
branch
from
4e699b0 to
9859e13
Compare
|
/test-all |
|
/test-all |
8 similar comments
|
/test-all |
|
/test-all |
|
/test-all |
|
/test-all |
|
/test-all |
|
/test-all |
|
/test-all |
|
/test-all |
| State: schema.ImportStatePassthrough, | ||
| }, | ||
| Schema: map[string]*schema.Schema{ | ||
| "member": { |
There was a problem hiding this comment.
Would it make more sense to implement the list as one resource (with list of paths inside?)
There was a problem hiding this comment.
We've considered - But then we'll run into the usual issue where this singleton NSX object is virtually indestructible using NSX API - TF doesn't really like these, I think. Is there any decent way to work around this?
There was a problem hiding this comment.
We can revert to default (empty list in this case?) when resource is destroyed - we do the same for vm tags, for example.
There was a problem hiding this comment.
We can - the question is if there is any advantage in handling this as a scalar, vs handling as a list which is more "natural" to terraform with full CRUD operation.
We've done this for context profile custom attributes and such.
There was a problem hiding this comment.
My personal feeling is that a single string does not justify a dedicated resource. But I don't insist.
As long as we're sure order is not important in this list, we can do it as suggested here
There was a problem hiding this comment.
@annakhm there is a more serious problem which I should look into:
When two members are created at the same time, there is a concurrency problem - both retrieve the member list, append the new member and when submitting one change is overwritten.
I'm not sure if we have some mean to handle this (and we might have this elsewhere maybe).
There was a problem hiding this comment.
I worked around the concurrency problem, and added a few test. I'd rather if you could have another look...
|
/test-all |
ksamoray
force-pushed
the
dfw_exclude_list
branch
2 times, most recently
from
bd98c8a to
0967f5d
Compare
|
/test-all |
ksamoray
force-pushed
the
dfw_exclude_list
branch
from
0967f5d to
f59c9aa
Compare
|
/test-all |
ksamoray
force-pushed
the
dfw_exclude_list
branch
from
f59c9aa to
da95fca
Compare
|
/test-all |
ksamoray
force-pushed
the
dfw_exclude_list
branch
3 times, most recently
from
0d8fe8f to
c2d2e9b
Compare
|
/test-all |
ksamoray
force-pushed
the
dfw_exclude_list
branch
from
c2d2e9b to
191fd95
Compare
|
/test-all |
|
|
||
| client := security.NewExcludeListClient(getSessionContext(d, m), connector) | ||
| obj, err := client.Get() | ||
| if isNotFoundError(err) { |
There was a problem hiding this comment.
We should silently swallow the error in not found case
|
I still feel like a singleton resource is better design, as it would save roundtrips and avoid concurrency problems. But I don't insist. |
ksamoray
force-pushed
the
dfw_exclude_list
branch
2 times, most recently
from
87b7bd3 to
bb191cf
Compare
|
/test-all |
IMO a singleton doesn't make sense in the API level as well, from the functional aspect, e.g:
|
|
/test-all |
|
@ksamoray shouldn't this PR also have doc changes since we are adding a new resource? Regarding the singleton vs non-singleton use case, I think both @annakhm and @ksamoray have good points. |
|
@salv-orlando yeah we need documentation but no point in adding that before we concluded the implementation. |
ksamoray
force-pushed
the
dfw_exclude_list
branch
from
bb191cf to
c29c928
Compare
Signed-off-by: Kobi Samoray <[email protected]>
ksamoray
force-pushed
the
dfw_exclude_list
branch
from
c29c928 to
2f963e8
Compare
|
/test-all |
Fixes: #757